fix(server): surface the declared 404 when deleting a missing message part - #1111
Conversation
… part The DELETE /session/:sessionID/message/:messageID/part/:partID route (part.delete) declares a 404, but Session.removePart was a silent no-op: it fired MessageV2.Event.PartRemoved and returned the id without checking existence, so deleting a part that does not exist returned 200. The declared 404 was a phantom. Add the existence check in the ROUTE handler (via Session.getPart, which returns the part or undefined) and throw NotFoundError when the part is missing (ErrorMiddleware maps it to 404). Unlike deleteMessage, the check cannot live in the service: Session.removePart is also called internally by the message processor (src/session/processor.ts), which must stay a tolerant no-op for an already-gone part. Keeping the check route-local makes the public contract real without changing the processor's behavior. No frontend impact: the desktop app does not call this endpoint directly (it reacts to the message.part.removed SSE event). The only behavior change is that deleting a missing/already-removed part now returns the already-declared 404. Refs #936
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 29 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary
Part of the staged Effect error-contract migration (#936): turn a silent 200
into the route's already-declared 404. Follows #1109, which did the same for
session.deleteMessage.DELETE /session/:sessionID/message/:messageID/part/:partID(part.delete)declares
errors(400, 404), butSession.removePartwas a silent no-op: itfired
MessageV2.Event.PartRemovedand returned the id without checkingexistence, so deleting a part that does not exist returned
200. The declared404 was a phantom.
Change
Add the existence check in the route handler (via
Session.getPart, whichreturns the part or
undefined) and throwNotFoundErrorwhen the part ismissing —
ErrorMiddlewaremaps it to 404 (and does not error-log it).Why route-local, not in the service (unlike #1109):
Session.removePartisalso called internally by the message processor (
src/session/processor.ts:1295),which must stay a tolerant no-op for an already-gone part. Putting the check in
removePartwould change the processor's behavior. Keeping it in the route makesthe public contract real with a contained blast radius.
Behavior change & de-risk
The only change is that deleting a missing / already-removed part now returns the
already-declared 404 instead of a misleading 200. The desktop app does not
call this endpoint directly — it reacts to the
message.part.removedSSE event —so there is no frontend impact. The processor path is untouched (full session
suite stays green).
Verification
bun test test/server/session-messages.test.ts— 12 pass (the existing part update/delete test now also asserts the 404 on a re-delete)bun test test/server/— 332 passbun test test/session/— 790 pass / 0 fail (confirms the processor's internalremovePartuse is unaffected)bun run typecheck— cleanDeferred (known, tracked)
The checked-in
packages/sdk/openapi.jsonsnapshot /src/v2/gentypes areregenerated in a single batched resync PR at the end of the #936 series, not
per-slice. No functional impact (the app reacts to SSE, not this response).
Refs #936